fix(react): clear removed snapshot prop refs#2590
Conversation
🦋 Changeset detectedLatest commit: 3ebe048 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughAdds helpers to find removed snapshot nodes and clears compiler-transient ChangesTransient child prop cleanup on snapshot removal
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Merging this PR will degrade performance by 21.72%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | 002-hello-reactLynx-destroyBackground |
670.3 µs | 916.9 µs | -26.9% |
| ❌ | 008-many-use-state-destroyBackground |
8 ms | 9.5 ms | -16.17% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing Yradex:wt/run-on-background-clear-prop-refs (3ebe048) with main (66f6ecf)
Footnotes
-
26 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
React External#1321 Bundle Size — 695.33KiB (+0.33%).3ebe048(current) vs 66f6ecf main#1314(baseline) Bundle metrics
Bundle size by type
Bundle analysis report Branch Yradex:wt/run-on-background-clea... Project dashboard Generated by RelativeCI Documentation Report issue |
React Example with Element Template#473 Bundle Size — 199.83KiB (0%).3ebe048(current) vs 66f6ecf main#466(baseline) Bundle metrics
Bundle size by type
|
| Current #473 |
Baseline #466 |
|
|---|---|---|
145.76KiB |
145.76KiB |
|
54.08KiB |
54.08KiB |
Bundle analysis report Branch Yradex:wt/run-on-background-clea... Project dashboard
Generated by RelativeCI Documentation Report issue
Web Explorer#9781 Bundle Size — 901.38KiB (0%).3ebe048(current) vs 66f6ecf main#9775(baseline) Bundle metrics
Bundle size by type
|
| Current #9781 |
Baseline #9775 |
|
|---|---|---|
497.1KiB |
497.1KiB |
|
402.06KiB |
402.06KiB |
|
2.22KiB |
2.22KiB |
Bundle analysis report Branch Yradex:wt/run-on-background-clea... Project dashboard
Generated by RelativeCI Documentation Report issue
React MTF Example#1340 Bundle Size — 208.1KiB (+0.31%).3ebe048(current) vs 66f6ecf main#1333(baseline) Bundle metrics
Bundle size by type
Bundle analysis report Branch Yradex:wt/run-on-background-clea... Project dashboard Generated by RelativeCI Documentation Report issue |
React Example#8207 Bundle Size — 237.15KiB (+0.27%).3ebe048(current) vs 66f6ecf main#8200(baseline) Bundle metrics
Bundle size by type
Bundle analysis report Branch Yradex:wt/run-on-background-clea... Project dashboard Generated by RelativeCI Documentation Report issue |
d81fa3c to
2354303
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/react/runtime/src/snapshot/snapshot/snapshot.ts`:
- Around line 74-83: clearTransientChildPropRefs currently retraverses the
removed subtree via traverseSnapshotInstance, causing O(n²) behavior when
callers repeatedly call clearTransientChildPropRefs(v, v); refactor to accept a
precomputed WeakSet of removed snapshots (e.g., add an optional parameter
removedSnapshots: WeakSet<object>) or an overload so callers can compute
removedSnapshots once using traverseSnapshotInstance(removedChild, ...) and pass
it in; update clearTransientChildPropRefs(owner, removedChild,
removedSnapshots?) to use the passed set if present (fall back to current
traversal only if not provided) and change the current call sites that do
clearTransientChildPropRefs(v, v) to compute the removedSnapshots once and pass
it in, preserving existing behavior when not provided.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bec8ee41-a6e0-4765-a7a9-26704fb7ebee
📒 Files selected for processing (3)
.changeset/clear-snapshot-prop-refs.mdpackages/react/runtime/__test__/snapshot/renderToOpcodes.test.jsxpackages/react/runtime/src/snapshot/snapshot/snapshot.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/react/runtime/__test__/snapshot/renderToOpcodes.test.jsx (1)
111-122: 💤 Low valueConsider testing deeper nesting levels.
The test verifies 2-level nesting (
[[child]]), which is good. Since the PR description mentions recursive handling of array shapes, consider adding a test case with 3+ levels of nesting (e.g.,[[[child]]]) to more thoroughly verify the recursive cleanup logic handles arbitrary depth.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react/runtime/__test__/snapshot/renderToOpcodes.test.jsx` around lines 111 - 122, Add a parallel test that exercises deeper nesting (e.g., use createElement('view', { $0: [[[child]]] }) and run renderToString(vnode, new SnapshotInstance('root'))), then call vnode.removeChild(child) and assert that vnode.props.$0 becomes the same shape with the child replaced by undefined (e.g., [[[undefined]]]) and vnode.childNodes is empty; mimic the existing test block ("should clear removed children from nested transient prop arrays") but with three or more nested array levels to verify recursive cleanup in removeChild and the transient-props handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/react/runtime/__test__/snapshot/renderToOpcodes.test.jsx`:
- Around line 111-122: Add a parallel test that exercises deeper nesting (e.g.,
use createElement('view', { $0: [[[child]]] }) and run renderToString(vnode, new
SnapshotInstance('root'))), then call vnode.removeChild(child) and assert that
vnode.props.$0 becomes the same shape with the child replaced by undefined
(e.g., [[[undefined]]]) and vnode.childNodes is empty; mimic the existing test
block ("should clear removed children from nested transient prop arrays") but
with three or more nested array levels to verify recursive cleanup in
removeChild and the transient-props handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 37d18710-3f8c-4aa0-96f4-8d5b5ba0c8db
📒 Files selected for processing (2)
packages/react/runtime/__test__/snapshot/renderToOpcodes.test.jsxpackages/react/runtime/src/snapshot/snapshot/snapshot.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react/runtime/src/snapshot/snapshot/snapshot.ts
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @lynx-js/autolink-codegen@0.1.0 ### Minor Changes - Add the Native Autolink codegen package. ([#2601](#2601)) ## create-lynx-extension@0.1.0 ### Minor Changes - Add the Native Autolink create-extension package. ([#2587](#2587)) ### Patch Changes - Use published package versions for scaffolded autolink codegen dependencies instead of workspace placeholders. ([#2628](#2628)) - Fix npm bin symlink entrypoint detection for the create extension CLI. ([#2623](#2623)) ## @lynx-js/react@0.121.0 ### Minor Changes - Support `React.createElement(type, props, children)` API. ([#2360](#2360)) ```jsx React.createElement("view", { style }, <text>hello</text>); // equivalent to <view style={style}> <text>hello</text> </view>; React.createElement(MyComponent, { style }, <view />); // equivalent to <MyComponent style={style}> <view /> </MyComponent>; ``` ### Patch Changes - Clear transient snapshot child props when removed snapshot subtrees are detached, preventing compiled `$*` child references from retaining deleted list holder or list item subtrees after removal. ([#2590](#2590)) - Add `createPortal` for rendering a subtree into a different ReactLynx element identified by a `NodesRef`. ([#2543](#2543)) ```tsx function App() { const [host, setHost] = useState(null); return ( <view> <view ref={setHost} /> {host && createPortal(<text>hi</text>, host)} </view> ); } ``` - Default `fireEvent` to `bubbles: true` for the TouchEvent family in testing-library to match Lynx runtime semantics, and stop reassigning the read-only `Event.prototype` accessors which threw `TypeError` in strict mode. ([#2532](#2532)) - Set `bundle-url` on lazy bundle border elements. ([#2537](#2537)) - Stop warning when `runWorklet` receives an invalid or missing main-thread function object. Invalid worklet contexts are still ignored, but nullish handler values no longer produce noisy `MainThreadFunction: Invalid function object` console output. ([#2586](#2586)) - Retain main-thread worklet context references before offscreen snapshot elements are materialized, so event, ref, gesture, and spread callbacks stay alive until the DOM update path can attach them. ([#2592](#2592)) - Update the @lynx-js/tasm dependency to 0.0.39 and align React template attribute descriptors with it. ([#2643](#2643)) - Avoid retaining transformed nested worklet contexts after worklet transformation. ([#2591](#2591)) Nested worklets transformed by the worklet runtime now keep their context recovery metadata through a weak reference, preventing cached transformed worklet functions from keeping list-item worklet contexts alive. ## @lynx-js/docs-mcp-server@0.2.3 ### Patch Changes - fix(docs-mcp): recursively crawl and register nested llms.txt resources ([#2317](#2317)) ## @lynx-js/rspeedy@0.14.4 ### Patch Changes - feat(qrcode): support get entry from api exposed from rspeedy.env.entries ([#2551](#2551)) - Updated dependencies \[[`ad1f90f`](ad1f90f)]: - @lynx-js/chunk-loading-webpack-plugin@0.3.4 - @lynx-js/web-rsbuild-server-middleware@0.20.4 - @lynx-js/cache-events-webpack-plugin@0.0.3 ## @lynx-js/lynx-bundle-rslib-config@0.3.3 ### Patch Changes - Update the @lynx-js/tasm dependency to 0.0.39 and align React template attribute descriptors with it. ([#2643](#2643)) ## @lynx-js/qrcode-rsbuild-plugin@0.4.7 ### Patch Changes - feat(qrcode): support get entry from api exposed from rspeedy.env.entries ([#2551](#2551)) ## @lynx-js/react-rsbuild-plugin@0.16.2 ### Patch Changes - Updated dependencies \[[`3e627b3`](3e627b3), [`7b8d63c`](7b8d63c), [`13a0776`](13a0776), [`a973c54`](a973c54), [`353b1b7`](353b1b7)]: - @lynx-js/template-webpack-plugin@0.11.1 - @lynx-js/react-refresh-webpack-plugin@0.3.6 - @lynx-js/react-alias-rsbuild-plugin@0.16.2 - @lynx-js/use-sync-external-store@1.5.0 - @lynx-js/react-webpack-plugin@0.9.2 - @lynx-js/css-extract-webpack-plugin@0.7.1 ## @lynx-js/web-core@0.20.4 ### Patch Changes - Always clone touch event lists when creating cross-thread events so synthetic touch events only carry structured-clone-safe primitive fields. ([#2636](#2636)) - Conditionally pass Card and Component params based on cardType in background thread. ([#2610](#2610)) - Add bidirectional decode worker heartbreak liveness messages. ([#2599](#2599)) - Add web support for the `<frame>` element by mapping it to `<lynx-view>`. ([#2604](#2604)) - Stop redeclaring `fetch` as a chunk-scope binding. Reusing the host ([#2562](#2562)) `window.fetch` from BTS chunks (instead of capturing the no-op stub the chunk wrapper used to install) lets the renderer issue real network requests. - Updated dependencies \[[`c1db603`](c1db603)]: - @lynx-js/web-elements@0.12.2 - @lynx-js/web-worker-rpc@0.20.4 ## @lynx-js/web-elements@0.12.2 ### Patch Changes - fix: xmarkdown create img incorrectly ([#2540](#2540)) ## @lynx-js/chunk-loading-webpack-plugin@0.3.4 ### Patch Changes - Override `__webpack_require__.e` so a single sync-then chunk load (the ([#2597](#2597)) typical lazy bundle case) bypasses `Promise.all`. It will make first screen in main thread can load lazy bundle synchronously when using dynamic import. ## @lynx-js/react-refresh-webpack-plugin@0.3.6 ### Patch Changes - Widen `@lynx-js/react-webpack-plugin` peer range to include `^0.9.0`. ([#2626](#2626)) ## @lynx-js/template-webpack-plugin@0.11.1 ### Patch Changes - feat(web): enable web binary template by default ([#2545](#2545)) The default encoding format for the web platform template has been changed from JSON to Binary. **Benefits for developers:** - **Smaller output size:** Binary templates are more compact than JSON strings, reducing the final bundle size. - **Faster load performance:** Binary templates parse faster than JSON in the runtime, improving the time-to-interactive for web applications. **How to turn off this feature:** If you encounter any issues with the new binary template format, you can revert to the previous JSON format by setting the environment variable `EXPERIMENTAL_USE_WEB_BINARY_TEMPLATE` to `'false'` or `'0'` before running your build commands. For example: `EXPERIMENTAL_USE_WEB_BINARY_TEMPLATE=false rspeedy build` **Upgrade to `@lynx-js/web-core@0.20.2` could support the new output format** See [`@lynx-js/web-core` Changelog](https://lynx-stack.dev/changelog/lynx-js--web-core) - Run TASM template encoding in a shared `tinypool` worker pool so multi-entry builds encode in parallel and watch-mode rebuilds reuse warm workers. ([#2634](#2634)) - Make `LynxTemplatePlugin.getLynxTemplatePluginHooks` a cross-module singleton so duplicate copies of this package (e.g. from npm hoist conflicts) share the same hooks per compilation. ([#2624](#2624)) - Update the @lynx-js/tasm dependency to 0.0.39 and align React template attribute descriptors with it. ([#2643](#2643)) - Updated dependencies \[[`ee79eff`](ee79eff), [`ded4de9`](ded4de9), [`cf01e94`](cf01e94), [`b989c1c`](b989c1c), [`8417e68`](8417e68)]: - @lynx-js/web-core@0.20.4 ## @lynx-js/react-umd@0.121.0 ## create-rspeedy@0.14.4 ## @lynx-js/react-alias-rsbuild-plugin@0.16.2 ## upgrade-rspeedy@0.14.4 ## @lynx-js/web-rsbuild-server-middleware@0.20.4 ## @lynx-js/web-worker-rpc@0.20.4 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Summary by CodeRabbit
Bug Fixes
Tests
Summary
This fixes a main-thread snapshot retention path where removed child subtrees could still be strongly referenced by compiler-generated transient child props. Snapshot JSX uses
$*props as staging references for named children; whenSnapshotInstance.removeChild()detached a child, the structural tree and native elements were cleaned up, but those$*props could still point at the removed snapshot subtree.The practical failure mode is that a deleted list holder or list item subtree can remain reachable from the root props chain after it is removed. That keeps old snapshot instances alive even though they are no longer part of the rendered tree.
Key points
Clear transient
$*owner props during removal. Direct refs such as{ $0: removedChild }are deleted fromprops, so the owner no longer keeps a hard reference to the removed subtree afterremoveChild().Handle compiled array child shapes recursively. When a transient prop stores children as arrays, only removed snapshot entries are cleared, including nested arrays:
Clean transient props inside the removed subtree as it is torn down. This breaks references held by the deleted subtree itself, including component/list structures that had their own compiled child staging props.
Runtime Contract
This only affects compiler-owned transient child props whose keys start with
$. Normal user props, public runtime APIs, serialized snapshot shape, native patch operations, and list update protocols are unchanged.The cleanup is tied to
SnapshotInstance.removeChild(): removed snapshot instances and descendants are collected into a removal set, then$*prop values are cleared if they directly reference any removed snapshot or contain one inside an array. Existing array identity is preserved, and non-removed entries keep their current identity and order.Checklist